More precise error messages #301

mamazu · 2018-08-27T20:42:13Z

As I have already mentioned (#296) I would suggest to use more precise error codes in the address book api to indicate for example that the user is not logged in. I have also added a PHPSpec test for the CurrentlyLoggedInUserProvider

mamazu · 2018-08-30T11:45:01Z

Please rerun the build.

lchrusciel · 2018-08-31T10:26:25Z

doc/swagger.yml

            400:
              description: "Validation failed"
              schema:
                  $ref: "#/definitions/GeneralError"
+            401:
+              description: "No user is logged in"
            500:


It should be 404, I guess

As you may have noticed I have added authorization symbols to the documentation. And the address book is something that only logged in users can see. If you are not logged in and try to call a route where you need to be logged in I wanted to return a 401 Unauthorized.

I suppose it is not the scope of this PR anyway?

The change in the documentation? That is already implemented. But the only parts that are behind a login wall are the address book and some of the customer endpoints.

Sorry, I was just confused, that we are handling error with 500 code. But this is totally not part of this PR. I'm just missing some part why, but this is rather general question about current codebase :)

True, the problem with the API was that assertions throw exceptions which are not very helpful to a user.

lchrusciel · 2018-08-31T10:27:17Z

src/Controller/AddressBook/CreateAddressAction.php

     */
-    private $tokenStorage;
+    private $currentUserProvider;


We should adjust the name of field as well

lchrusciel · 2018-08-31T10:27:56Z

src/Controller/AddressBook/RemoveAddressAction.php

-     * @param CommandBus $bus
-     * @param TokenStorageInterface $tokenStorage
-     * @param CurrentUserProviderInterface $currentUserProvider
+     * @param ViewHandlerInterface          $viewHandler


No doc comment variable alignment?

We have resigned from this practice, as it is problematic once you want to add a new argument to docblock. People tend to forget about alignment and even if not, then diffs are less readable. Can you revert these changes?

Only manually, but I will.

lchrusciel · 2018-08-31T10:29:22Z

tests/Controller/AddressBookShowApiTest.php

+        $this->client->request('GET', '/shop-api/address-book', [], [], ['ACCEPT' => 'application/json']);
+        $response = $this->client->getResponse();
+
+        $this->assertResponse($response, 'address_book/show_address_book_unauthorized_response', Response::HTTP_UNAUTHORIZED);


Can't we just assert response code?

lchrusciel · 2018-08-31T16:33:30Z

src/Validator/AddressExistsValidator.php

     */
    private $currentUserProvider;

    /**
     * AddressExistsValidator constructor.
     *
-     * @param AddressRepositoryInterface $addressRepository
-     * @param CurrentUserProviderInterface $currentUserProvider
+     * @param AddressRepositoryInterface    $addressRepository


Should be reversed

lchrusciel · 2018-08-31T16:33:43Z

src/Validator/AddressExistsValidator.php

@@ -18,19 +18,19 @@
     */
    private $addressRepository;
    /**
-     * @var CurrentUserProviderInterface
+     * @var LoggedInUserProviderInterface
     */
    private $currentUserProvider;


We should rename this parameter in the whole codebase

lchrusciel · 2018-08-31T16:33:58Z

src/Request/RemoveAddressRequest.php

@@ -9,12 +9,20 @@
 final class RemoveAddressRequest
 {
    /**
-     * @var mixed
+     * @var int|string


lchrusciel · 2018-08-31T16:34:06Z

src/Provider/LoggedInUserProvider.php

 {
    /**
-     * @var TokenStorage


lchrusciel · 2018-08-31T16:34:47Z

src/Controller/Customer/UpdateCustomerAction.php

@@ -77,9 +77,11 @@ public function __invoke(Request $request): Response
        $updateCustomerCommand = $updateCustomerRequest->getCommand();
        $this->bus->handle($updateCustomerCommand);

-        return $this->viewHandler->handle(View::create(
+        return $this->viewHandler->handle(
+            View::create(


Should be reverted or arguments of create method should be indented

lchrusciel · 2018-08-31T16:38:29Z

src/Controller/AddressBook/UpdateAddressAction.php

+                $this->addressBookViewFactory->create($updatedAddress, $user->getCustomer()),
+                Response::HTTP_OK
+            );
+        } else {


Instead of else we should use early return approach. Using return $this->viewHandler->handle(...); with two different arguments should be easier to understand what is happening here

lchrusciel · 2018-08-31T16:39:26Z

src/Controller/AddressBook/CreateAddressAction.php

+            return $this->viewHandler->handle(View::create(null, Response::HTTP_UNAUTHORIZED));
+        }
+
+        if (($customer = $user->getCustomer()) !== null) {


Not needed doubled brackets

lchrusciel · 2018-08-31T16:39:47Z

spec/Provider/LoggedInUserProviderSpec.php

+        TokenStorageInterface $tokenStorage,
+        TokenInterface $token,
+        ShopUserInterface $shopUser
+    )


lchrusciel · 2018-08-31T16:41:28Z

Thanks @mamazu! 🐋

mamazu added 5 commits August 27, 2018 18:57

Added more precise response codes

7797a08

Updated the documentaion and unified the responses

9ca1eba

Added return value to create action

56aa0a4

Changed response codes

a5767f4

Added documentation

2bfb722

mamazu added 2 commits August 31, 2018 09:50

Codestyle

a4c5fcc

Added codestyle

450ad57

lchrusciel reviewed Aug 31, 2018

View reviewed changes

mamazu added 2 commits August 31, 2018 13:03

Implemented suggestions (renamed variables)

61a1eba

Unaligned parameters

c701680

lchrusciel approved these changes Aug 31, 2018

View reviewed changes

lchrusciel merged commit 4a51f18 into Sylius:master Aug 31, 2018

mamazu mentioned this pull request Aug 31, 2018

Returning more precise error codes #296

Closed

mamazu deleted the more_precise_error_messages branch September 12, 2018 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More precise error messages #301

More precise error messages #301

mamazu commented Aug 27, 2018

mamazu commented Aug 30, 2018

lchrusciel Aug 31, 2018

mamazu Aug 31, 2018

lchrusciel Aug 31, 2018

mamazu Aug 31, 2018

lchrusciel Aug 31, 2018

mamazu Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

mamazu Aug 31, 2018

lchrusciel Aug 31, 2018

mamazu Aug 31, 2018

lchrusciel Aug 31, 2018

mamazu Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel Aug 31, 2018

lchrusciel commented Aug 31, 2018

More precise error messages #301

More precise error messages #301

Conversation

mamazu commented Aug 27, 2018

mamazu commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchrusciel commented Aug 31, 2018